Display number of unread messages as a badge on the jump-to-bottom button. Send fully-read receipts more efficiently. #206

alanpoon · 2024-10-15T04:09:55Z

Implemented: Start a 5-second timeout timer whenever scrolling stops, and then after the timer has reached its timeout deadline, send the read receipts for the last visible event.
Modified Send fully Receipt condition to be executed only after scrolling past Fully Read Marker
Added obtaining Fully read marker's event_id Request

Added Jump to Top view
Added Jump to bottom badge counter

scrollup2.mp4

kevinaboos

See my comments about the Jump to Top (JTT) button -- we're not yet ready to properly implement that, so I think it should be removed, or at least hidden until we can fix it.

Everything else looks fine, thanks!

resources/icon_jump_to_top.svg

src/home/room_screen.rs

src/sliding_sync.rs

src/home/room_screen.rs

src/sliding_sync.rs

src/home/room_screen.rs

alanpoon · 2024-11-06T10:12:46Z

Currently there's a bug in the portallist's is_at_end function which returns true even when the scroll is not at the bottom. Hence the JTB will not show.
makepad/makepad#579

kevinaboos · 2024-11-25T21:22:08Z

Curr

I've redesigned how the jump to bottom button works to make it easier to use. See #250 for the details, or just checkout the jump_to_bottom_button.rs file.

Currently, the portal_list.smooth_scroll_to_end is not terminating, let me investigate

I've fixed this as of last week; I rewrote the entire smooth_scroll_to() function here: makepad/makepad#600

kevinaboos

Nice, the code is looking better and is certainly a lot more efficient and easier to understand!

It's good that you're querying the user's latest fully read state when you show the room timeline for the first time. However, that approach will only catch changes to the user's read receipt once, when the room is first shown. Imagine the following scenario:

The user opens a room in Robrix
Robrix is open, but sitting idle on their computer
The user opens another client, e.g., on their phone
New messages arrive for that room
The user sees those new messages and interacts with them on their phone

Then, when the user returns to Robrix, the latest fully read receipt for that room will be wrong/outdated, because it didn't get updated while the user was interacting with their phone.

I think we actually need to subscribe to this state change such that we can know if it has changed on other devices. (We can do this in a future PR, but if it's easy then we should just do it now.)

src/home/room_screen.rs

src/sliding_sync.rs

kevinaboos · 2024-11-25T22:21:32Z

src/shared/jump_to_bottom_button.rs

+        if unread_message_count > 0 {
+            self.visible = true;
+            self.view(id!(unread_message_badge)).set_visible(true);
+            self.label(id!(unread_messages_count)).set_text(&format!("{}", unread_message_count));


we should probably limit it to a certain number of events such that the label doesn't look ridiculous if there are 10000000+ unread events.

We could do something like up to two digits (a max value of 99) or three digits (a max value of 999) until we just show 99+ or 999+. Depends on how wide the label can be while still looking good.

Use your best judgment.

Limited to 99+

Nice, looks pretty good. The circular background might need to be an oval/ellipse, since we need more padding on the left and right of the 99+ string to make it look more professional and readable.

I think if you set the width of the parent view of the label to be Fit, with a small amount of left & right padding, that should do it.

Improved unread badge count

Co-authored-by: Kevin Boos <[email protected]>

…to send_read+jtb

…send_read+jtb

src/sliding_sync.rs

alanpoon · 2024-12-03T07:06:56Z

Nice, the code is looking better and is certainly a lot more efficient and easier to understand!

It's good that you're querying the user's latest fully read state when you show the room timeline for the first time. However, that approach will only catch changes to the user's read receipt once, when the room is first shown. Imagine the following scenario:

The user opens a room in Robrix

Robrix is open, but sitting idle on their computer

The user opens another client, e.g., on their phone

New messages arrive for that room

The user sees those new messages and interacts with them on their phone

Then, when the user returns to Robrix, the latest fully read receipt for that room will be wrong/outdated, because it didn't get updated while the user was interacting with their phone.

I think we actually need to subscribe to this state change such that we can know if it has changed on other devices. (We can do this in a future PR, but if it's easy then we should just do it now.)

Added subscribe_to_updates mechanism to update the latest read receipt in multiple devices.

kevinaboos

In general, I think this looks pretty good. I left a few small comments, but I have one major request: let's just remove the concept of the 5-second timer. It's too complicated and will probably just confuse users. Let's just send the read receipt based on the user's scrolling actions, which should also make the code simpler and more efficient.

I think removing the timer is also consistent with what power users expect. For example, sometimes I just want to quickly skim the latest messages from 10 different rooms and consider those as fully read. That process should only take me a few seconds, not over 50 seconds (5 seconds for each room).

Also, I still don't think that storing the latest read receipt event in the ALL_ROOM_INFO map is the correct way to do things. It is inefficient, requires undesirable locking, and it doesn't even seem to be necessary.

Thus, let's consider how we could improve this. It looks like that info is only used within the RoomScreen, and it is a form of per-room data. Since we're already sending TimelineUpdates to the RoomScreen widget from the backend update subscriber, why wouldn't we just keep that information directly within each room's UI state? That way, we avoid locking, we avoid the overhead of adding and updating that info within the ALL_ROOM_INFO hashmap, and we avoid the memory usage of storing it in multiple different places.

What do you think about that approach? If you like it, please make the changes to follow that design instead.

kevinaboos · 2024-12-16T20:03:12Z

src/home/room_screen.rs

+    /// Timer to send fully read receipt
+    /// 
+    /// 5 secs timer starts when user scrolled down and scrolled_past_read_marker is true
+    /// Last displayed event will be sent as fully read receipt


pro-tip: doc comments like this get rendered into Html, so you need to format them as full sentences rather than just sentence fragments across line breaks.

Suggested change

/// Timer to send fully read receipt

///

/// 5 secs timer starts when user scrolled down and scrolled_past_read_marker is true

/// Last displayed event will be sent as fully read receipt

/// Timer used to send fully-read receipts for the current room.

///

/// We start a short (5 second) timer when the user scrolls down,

/// if `scrolled_past_read_marker` is true. Then, we send a fully-read receipt

/// for the last displayed event in this room.

kevinaboos · 2024-12-16T20:06:02Z

src/home/room_screen.rs

-                        *moved_to_queue = true;
-                    }
+        // Send fully read receipt for last displayed event 5 Seconds after user scrolled past the read marker
+        if self.fully_read_timer.is_event(event).is_some() && !self.fully_read_timer.is_empty() {


can you explain (and add a comment) why it's necessary to check that the fully_read_timer is not empty?

Also, what does it mean for a timer to be non-empty?

kevinaboos · 2024-12-16T20:13:21Z

src/home/room_screen.rs

+                    }
+                    if let Some(unread_messages_count) = unread_messages_count {
+                        jump_to_bottom.show_unread_message_badge(cx, unread_messages_count);
+                        continue;


why continue here? that seems odd, as we typically wouldn't want to skip anything else beneath this point.

kevinaboos · 2024-12-16T20:23:42Z

src/shared/jump_to_bottom_button.rs

+                height: Fit,
+                padding: { left: 0.0, right: 2.0 }
+                text: "",
+


this unread messages label is not centered horizontally with respect to the jump-to-bottom button. You can try align: {x: 0.5} here, but I think you'll also need more than that change to make it perfectly centered.

The right-side padding of 2.0 will also make things look off-center too.

kevinaboos · 2024-12-16T20:30:22Z

src/sliding_sync.rs

@@ -287,6 +288,14 @@ pub enum MatrixRequest {
        /// * If `false` (recommended), details will be fetched from the server.
        local_only: bool,
    },
+    /// Request to fetch the fully read event in the given room's timeline.


Suggested change

/// Request to fetch the fully read event in the given room's timeline.

/// Request to fetch the user's latest fully-read event for the given room.

kevinaboos · 2024-12-16T20:35:52Z

src/sliding_sync.rs

+                            if let Some(latest_active) = serde_json::to_value(room.read_receipts())
+                                .unwrap_or_default()
+                                .get("latest_active")
+                                .unwrap_or(&serde_json::Value::Null)
+                                .get("event_id") {


this is unreadable without proper formatting

Suggested change

if let Some(latest_active) = serde_json::to_value(room.read_receipts())

.unwrap_or_default()

.get("latest_active")

.unwrap_or(&serde_json::Value::Null)

.get("event_id") {

if let Some(latest_active) = serde_json::to_value(room.read_receipts())

.unwrap_or_default()

.get("latest_active")

.unwrap_or(&serde_json::Value::Null)

.get("event_id")

{

kevinaboos · 2024-12-16T20:37:35Z

src/sliding_sync.rs

+                        Ok(sent) => log!("{} send fully read receipt to room {room_id} for event {event_id}", 
+                            if sent { "Sent" } else { "Already sent" }
+                        ),


Suggested change

Ok(sent) => log!("{} send fully read receipt to room {room_id} for event {event_id}",

if sent { "Sent" } else { "Already sent" }

),

Ok(sent) => log!("{} fully read receipt to room {room_id} for event {event_id}",

if sent { "Sent" } else { "Already sent" }

),

kevinaboos · 2024-12-16T20:38:07Z

src/sliding_sync.rs

@@ -884,8 +1015,12 @@ struct RoomInfo {
    timeline_subscriber_handler_task: JoinHandle<()>,
    /// A drop guard for the event handler that represents a subscription to typing notices for this room.
    typing_notice_subscriber: Option<EventHandlerDropGuard>,
+    /// A broadcast receiver for room updates.


incorrect comment, this is not a receiver, it is boolean.

kevinaboos · 2024-12-16T20:40:48Z

src/home/room_screen.rs

@@ -1877,6 +1892,9 @@ impl RoomScreen {
            }
        );

+        // Query for User's fully read event
+        submit_async_request(MatrixRequest::GetFullyReadEvent { room_id: room_id.clone() });
+        submit_async_request(MatrixRequest::SubscribeToUpdates { room_id: room_id.clone(), subscribe: true });


and do you ever unsubscribe from these updates?

kevinaboos · 2024-12-16T20:49:37Z

src/sliding_sync.rs

+                            // Using Serde_json to obtain the latest_active event_id  
+                            // Room's read receipt's latest_active event is not public, but the ReadReceipt struct 
+                            // is serializable 
+                            if let Some(latest_active) = serde_json::to_value(room.read_receipts())


surely there is an easier way to accomplish this?

Check out the docs for both Room and Timeline -- there are several methods related to getting the latest read receipt for a given user.
For example, here's one to get you started: https://matrix-org.github.io/matrix-rust-sdk/matrix_sdk_ui/timeline/struct.Timeline.html#method.latest_user_read_receipt

alanpoon added 14 commits October 4, 2024 12:31

reduce complexity of send_read_receipt

5657931

cleanup

bb92afb

removed unnecessary import

9255365

added unread_message_badge

24be468

remove unneccesary changes

c1d1ab6

add jump to top

35c5977

added unread_notification_badge

bc2aa8c

Merge branch 'main' into jtb_change_in_timeline_item#131

a9e4945

Merge branch 'main' into send_read_receipt_improve

053c8f0

Added scroll past marker

e4c1268

Merge branch 'jtb_change_in_timeline_item#131' into send_read+jtb

072b9cb

wip

3884a66

send read +jtb

b7a83a0

Merge branch 'main' into send_read+jtb

a3b1f10

kevinaboos requested changes Oct 30, 2024

View reviewed changes

alanpoon added 4 commits November 4, 2024 11:03

added some docs, remove icon_jump_to_top

3824160

Merge branch 'main' into send_read+jtb

5633e2b

merge .cargo

44f4c25

minor formating

9076c53

alanpoon commented Nov 4, 2024

View reviewed changes

src/home/room_screen.rs Outdated Show resolved Hide resolved

alanpoon added 5 commits November 4, 2024 21:22

change to start_timeout

5bf2413

Merge branch 'main' into send_read+jtb

1c66730

removed jump_to_top & fix timer issue

8bbde58

set read event into RoomInfo's fully read when sending

b99a57d

restructure scroll pass read marker logic

99ff1b1

ZhangHanDong requested a review from kevinaboos November 25, 2024 13:25

Code improvement n log improvement

963a722

kevinaboos requested changes Nov 25, 2024

View reviewed changes

kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Nov 25, 2024

alanpoon and others added 11 commits November 27, 2024 10:27

changed and_then to map

1566621

Co-authored-by: Kevin Boos <[email protected]>

changed to get_fully_read_event

3627a1f

Co-authored-by: Kevin Boos <[email protected]>

cap at 99+

07df9fd

Merge branch 'send_read+jtb' of https://github.com/alanpoon/robrix in…

b44105a

…to send_read+jtb

Merge branch 'main' of https://github.com/project-robius/robrix into …

c32b7d5

…send_read+jtb

change to get_fully_read_event

1e7d4b2

Move client num_of_unread to async

ced53e5

make oval for 99+

1caacc1

Merge branch 'main' of https://github.com/project-robius/robrix into …

22d8e30

…send_read+jtb

fix clippy warning

35b8b02

fix error handling for set_fully_read_event

906e53e

alanpoon added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Nov 28, 2024

aaravlu requested changes Nov 30, 2024

View reviewed changes

src/sliding_sync.rs Outdated Show resolved Hide resolved

alanpoon added 5 commits December 2, 2024 23:07

added subscribe to update read_receipt

fe9973b

Added comments for subscribe to Updates

3d090fb

added new lines and fix clippy

b12592a

added update_subscriber_initialised check

eb857c6

fix clippy

794cd0a

kevinaboos requested changes Dec 16, 2024

View reviewed changes

kevinaboos changed the title ~~Combine PR for Send read receipt #86 +jump button for scroll #131~~ Display number of unread messages as a badge on the jump-to-bottom button. Send fully-read receipts more efficiently. Dec 16, 2024

kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display number of unread messages as a badge on the jump-to-bottom button. Send fully-read receipts more efficiently. #206

Display number of unread messages as a badge on the jump-to-bottom button. Send fully-read receipts more efficiently. #206

alanpoon commented Oct 15, 2024 •

edited

Loading

kevinaboos left a comment

alanpoon commented Nov 6, 2024

kevinaboos commented Nov 25, 2024

kevinaboos left a comment

kevinaboos Nov 25, 2024

alanpoon Nov 27, 2024 •

edited

Loading

kevinaboos Nov 27, 2024

alanpoon Nov 28, 2024

alanpoon commented Dec 3, 2024

kevinaboos left a comment

kevinaboos Dec 16, 2024

kevinaboos Dec 16, 2024

kevinaboos Dec 16, 2024

kevinaboos Dec 16, 2024

kevinaboos Dec 16, 2024

kevinaboos Dec 16, 2024

kevinaboos Dec 16, 2024

kevinaboos Dec 16, 2024

kevinaboos Dec 16, 2024

kevinaboos Dec 16, 2024

kevinaboos Dec 16, 2024 •

edited

Loading

-    /// Timer to send fully read receipt
-    ///
-    /// 5 secs timer starts when user scrolled down and scrolled_past_read_marker is true
-    /// Last displayed event will be sent as fully read receipt
+    /// Timer used to send fully-read receipts for the current room.
+    ///
+    /// We start a short (5 second) timer when the user scrolls down,
+    /// if `scrolled_past_read_marker` is true. Then, we send a fully-read receipt
+    /// for the last displayed event in this room.

	/// Request to fetch the fully read event in the given room's timeline.
	/// Request to fetch the user's latest fully-read event for the given room.

Display number of unread messages as a badge on the jump-to-bottom button. Send fully-read receipts more efficiently. #206

Are you sure you want to change the base?

Display number of unread messages as a badge on the jump-to-bottom button. Send fully-read receipts more efficiently. #206

Conversation

alanpoon commented Oct 15, 2024 • edited Loading

kevinaboos left a comment

Choose a reason for hiding this comment

alanpoon commented Nov 6, 2024

kevinaboos commented Nov 25, 2024

kevinaboos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoon Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoon commented Dec 3, 2024

kevinaboos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinaboos Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

alanpoon commented Oct 15, 2024 •

edited

Loading

alanpoon Nov 27, 2024 •

edited

Loading

kevinaboos Dec 16, 2024 •

edited

Loading